-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Caching of decrypted data #46
Caching of decrypted data #46
Conversation
@@ -14,6 +14,9 @@ field.storage.*.*.third_party.field_encrypt: | |||
encryption_profile: | |||
type: string | |||
label: 'Encryption profile' | |||
cache_exclude: | |||
type: boolean | |||
label: 'Exclude from cache' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit ambiguous. We're only focusing on render cache, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's broader than just render cache. When caching the entity object itself, unencrypted field data also shouldn't be stored in the (database) cache...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think uncacheable
would be a better name. It'd also fit in better with the terminology in Drupal core.
… should be excluded from caching.
|
||
// If cache_exclude is set, set caching max-age to 0. | ||
if ($storage->getThirdPartySetting('field_encrypt', 'cache_exclude', TRUE) == TRUE) { | ||
$build[$field->getName()]['#cache']['max-age'] = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
TODO:
|
…an be extended when it lands in Encrypt module.
…ed in entity cache.
Extra logic and test added to fix caching on entity level, in 'cache.entity'. |
$form['field_encrypt']['field_encrypt']['cache_exclude'] = [ | ||
'#type' => 'checkbox', | ||
'#title' => t('Exclude from cache'), | ||
'#description' => t('Exclude this field from being cached. This will make sure your unencrypted data will not be exposed in the cache, but could have impact on your performance.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/could have impact/will have a negative impact/
I haven't studied all the commits here, but I wonder why a field would decrypt during entity load. I would decrypyt at render time and mark the render array as max-age=0. That way we dont have to worry about entity cache, as it stores encrypted value. |
@weitzman Great idea. But I'm not entirely sure that's going to be possible. If field values are encrypted at all times except rendering, then I can imagine that anything relying on field data won't work correctly: Views, Rules … This is also not how the module works currently, at least as far as I can tell based on https://raw.githubusercontent.com/d8-contrib-modules/field_encrypt/master/documentation/Field%20Encrypt%20Architecture.png. I wish https://github.com/d8-contrib-modules/field_encrypt/blob/master/readme.md explained the architecture in detail, and especially the rationale for going with this approach. It's sparsely documented. Which is alsow hy I had to ask #17 (comment) in the first place. |
@wimleers - I'm happy to elaborate on the architecture in greater detail if you can be more clear on gaps you have found. Do you want to know what hooks are being used? How we're storing things? Please share some of what you wish to see and I'll add it. And, yes, any sort of metadata or data used in the processing of other entities (blocks, views, etc) will break if we only do rendering. In fact, we tried it. |
Great, could you document in the README then why you did not go that way? That'd help address concerns that anybody evaluating this module will have. |
@nerdstein And yes, it would be great if you could comprehensively document how this module deals with:
i.e. any and all aspects affecting encryption, decryption and passing around decrypted data need to be documented. And actually… also how we're guaranteeing that the decryption keys are stored safely. EDIT: |
I will do so for the module as it is right now - but I'm very much open to adjusting the architecture if there is a better way to do this. That's precisely why we're asking for your contributions. Our previous attempts to be more comprehensive with this may have been off, but it might not have been implemented correctly either. If we have to make some architectural adjustments for this module - we should do it now. |
@wimleers: I have updated the code based on your feedback to set the cache properties on the EntityType. This works locally, but somehow the test case seems to cache the entity where it shouldn't. Still need to figure out why that assertion is failing exactly, as I can't reproduce it outside of the test. |
In the latest commit I have taken an alternative approach to determine which entity types should be uncacheable. This seems to work better, and now the test cases are all passing. Feedback welcome. |
@@ -84,6 +88,19 @@ function field_encrypt_form_alter(&$form, FormStateInterface $form_state, $form_ | |||
], | |||
); | |||
|
|||
// Add setting to decide if field should be excluded from cache. | |||
$form['field_encrypt']['field_encrypt']['uncacheable'] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Looks good to me! I'll be merging and i'll be doing extensive testing next week |
First draft to tackle the caching of encrypted data issue (see #17).
This PR: